-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Extend wrapped JS function coverage in core runtime types #12263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…ame JSError to ThrownFromJavaScript - Introduced new methods to core JS runtime types to improve host interop - Renamed JSError to ThrownFromJavaScript for semantic clarity - Original name implied alignment with native JS Error, but the class represents any thrown JS object
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
sdk/src/org.graalvm.webimage.api/src/org/graalvm/webimage/api/JSNumber.java
Outdated
Show resolved
Hide resolved
sdk/src/org.graalvm.webimage.api/src/org/graalvm/webimage/api/JSString.java
Outdated
Show resolved
Hide resolved
public static native JSString fromCharCode(); | ||
|
||
@JS.Coerce | ||
@JS(value = "return String.fromCharCode(...codeUnits);") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was conversion to a JS array not necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conversion wasn’t necessary in this case because primitive arrays are apparently coerced automatically. At least the int array is coerced to a Int32Array
in JavaScript in this function.
|
||
@JS.Coerce | ||
@JS(value = "return Symbol.keyFor(sym);") | ||
public static native JSValue keyFor(JSSymbol sym); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this return a JSString
always, so it could return a String
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could also return undefined
, which is why JSValue
is currently used here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case I think we should modify this to return a Java string and null
if the JS functions returned undefined
Thank you for signing the OCA. |
} else if (checkClass(JSSymbolTest.class, className)) { | ||
JSSymbolTest.main(null); | ||
} else if (checkClass(JSObjectTest.class, className)) { | ||
JSObjectTest.main(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass remainingArgs
for all these tests, even if the arguments are not used right now. Passing null
to the main
method is really unexpected
sdk/src/org.graalvm.webimage.api/src/org/graalvm/webimage/api/JSNumber.java
Show resolved
Hide resolved
|
||
@JS.Coerce | ||
@JS(value = "return parseFloat(number);") | ||
public native static float parseFloat(String number); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parseFloat
returns JS number, so we should use double
here.
Also, we should probably prefer Number.parseFloat
(even though they're the same function) just for consistency.
sdk/src/org.graalvm.webimage.api/src/org/graalvm/webimage/api/JSNumber.java
Show resolved
Hide resolved
public native static int parseInt(String number); | ||
|
||
@JS.Coerce | ||
@JS(value = "return parseInt(number, radix);") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer Number.parseInt
for consistency
|
||
@JS.Coerce | ||
@JS(value = "return Symbol.keyFor(sym);") | ||
public static native JSValue keyFor(JSSymbol sym); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case I think we should modify this to return a Java string and null
if the JS functions returned undefined
|
||
@JS.Coerce | ||
@JS(value = "return sym.valueOf();") | ||
public static native JSSymbol valueOf(Object sym); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this method needed?
JSSymbol sym = JSSymbol.asyncDispose(); | ||
|
||
assertEquals("JavaScript<symbol; Symbol(nodejs.asyncDispose)>", sym.toString()); | ||
assertEquals(JSSymbol.class, sym.getClass()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is check is not needed here or in any of the tests below. sym.getClass()
is always JSSymbol.class
since the type of the variable is JSSymbol
, which is a final class
|
||
@JS.Coerce | ||
@JS(value = "return sym.description;") | ||
public static native String description(Object sym); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not an instance method?
import static org.junit.Assert.assertNotSame; | ||
import static org.junit.Assert.assertTrue; | ||
|
||
public class JSSymbolTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to have tests for any of the well-known symbols. There doesn't seem to be a spec that defines what description
or keyFor
should produce for them. And just comparing against things like nodejs.asyncDispose
ties us to the behavior of a specific node version.
|
||
@JS.Coerce | ||
@JS(value = "return String.fromCharCode(...codeUnits);") | ||
public static native JSString fromCharCode(int... codeUnits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use use char... codeUnits
instead here? The function only accepts numbers from 0 to 65535 anyway.
|
||
@JS.Coerce | ||
@JS(value = "return String.fromCodePoint(...codeUnits);") | ||
public static native JSString fromCodePoint(int... codeUnits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static native JSString fromCodePoint(int... codeUnits); | |
public static native JSString fromCodePoint(int... codePoints); |
for (let i = 0; i < substitutions.length; i++) { | ||
args.push(substitutions[i]); | ||
} | ||
return String.raw(template, ...args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does ...Array.from(substitutions)
work?
|
||
@JS.Coerce | ||
@JS(value = "return this.localeCompare(compareString, locales, options);") | ||
public native int localeCompare(JSString compareString, JSString locales, Object options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be JSObject
here and above
|
||
@JS.Coerce | ||
@JS(value = "return this.match(regexp);") | ||
public native JSObject match(Object regexp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public native JSObject match(Object regexp); | |
public native JSObject match(JSObject regexp); |
|
||
@JS.Coerce | ||
@JS(value = "return Object.hasOwn(obj, prop);") | ||
public static native boolean hasOwn(JSObject obj, String prop); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create overloads for JSSymbol
and JSString
|
||
@JS.Coerce | ||
@JS(value = "return this.propertyIsEnumerable(prop);") | ||
public native boolean propertyIsEnumerable(String prop); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create overloads for JSSymbol
and JSString
try { | ||
obj.set("name", "Alice"); | ||
} catch (Exception e) { | ||
failed = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use assertThrows
here and in the rest of this class
} | ||
String result = JSValue.checkedCoerce(obj.get("name"), String.class); | ||
JSValue fun = (JSValue) obj.get("greet"); | ||
String greeting = apply(fun, null, "World"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this the same as in testPrototypeInheritance
? If so, this part can be removed from this test
for (let i = 0; i < values.length; i++) { | ||
arr.push(values[i]); | ||
} | ||
return arr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does return Array.from(values)
work here?
This pull request expands the set of host-accessible wrappers for internal JavaScript functions across key runtime types:
JSObject
,JSNumber
,JSString
,JSSymbol
, andJSValue
. These additions improve Java interop and simplify invocation of JavaScript behavior from host code.Additionally, the class
JSError
has been renamed toThrownFromJavaScript
to better reflect its actual role. The original name implied alignment with the native JavaScriptError
object, but the class more accurately represents any object thrown from JavaScript, regardless of its type.